Repository navigation
fix(runtime): free the admission slot when the deferred prelude panics - #138
Conversation
…eferred prelude Under DispatchDeferred, a State panic in the prelude runs on the net/http goroutine. The test serves the runtime behind a real httptest.Server, panics State.AcquireLock under the queue strategy and State.MarkEvent under the burst strategy, and requires that no queue turn record stays behind and that Shutdown drains before a 10s deadline. It fails on main: the panicked delivery keeps its admission slot, so Shutdown returns context deadline exceeded.
dispatchDeferred freed the slot only when the prelude returned. A State panic unwound past it to net/http, which recovers the panic, so the delivery kept its slot and its queue turn record, and Shutdown timed out. A deferred call now runs the release on every exit that hands no work to a tail, a panic included. The panic still propagates to net/http. The release is the sync.OnceFunc from #134, so no exit can release twice.
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…panics (#142) ## Why Every open follow-up issue in coder/chat gets one small, reviewed PR, and this one closes #139. When `State` or the app's `slog` handler panics during the prelude, `net/http` recovers the panic, but three things the delivery took stay taken. A redelivery of the event waits on the open acceptance entry until its own request ends. The next delivery for the thread waits for `ThreadLockTTL`. A burst member whose join log panicked never gets a runner, so later members never run and `Shutdown` times out. ## What changed - The prelude defers `abandonEventAcceptance` and its own `releaseOnResolve`. The first does nothing once the prelude finished the entry. The second does nothing once the lease is released or handed to the tail. So every release still runs once, and nothing recovers the panic. - `joinBurstBatch` starts the burst runner in a `defer`. The runner still starts after the join log, and it now also starts when the log panics. - `takeQueueTurn` deletes the superseded waiter before it logs. Without that, the released lease lets the old waiter take the lock behind the newer turn, and the `commitPending` assert ends the process under `DispatchDeferred`. ADR 0015 gains the rule as invariant 4. ## Scope - Covers `State` and logger panics in the prelude and in the burst join, under both dispatch modes. - A logger panic in the supersede log still loses events, because `MarkEvent` already ran and the platform's retry is deduped. Under debounce the newest event is lost. In `takeQueueTurn` both the newer event and the waiter it replaced are lost. That is event loss, not a leak, and is tracked in #143. - The dispatch span of a panicked delivery stays open, as on `main`. ## Tradeoffs - A `recover` and re-panic would also free each resource. The deferred releases leave the panic value and stack untouched, as #138 chose. - Moving `go runBurstScope` above the join log would also fix the burst leak, but the runner could then log before the member's join line. ## Blast Radius Only the prelude, the burst join, and the queue supersede path change. On every normal path the new defers find nothing left to release. ## Verification - Each fix lands after its failing test. Before the fixes, the redelivery and the next delivery for the thread time out after 2 s, and the burst member never runs. - Wrong fixes fail the new tests. A silent `recover` returns 200 or 500 instead of aborting the request. A second release reports `lock_release_failed` or panics with `close of closed channel`. A release after the handoff fails the existing lock tests, or admits a second burst member while the first holds the only slot. The lease release without the `takeQueueTurn` change trips the `commitPending` assert. - The new tests and the Queue, Deferred, Burst, Admission, and Concurrent tests pass 20 of 20 runs under `-race`. `go vet` and `go test` pass in every workspace module, and `go test -race` passes for the root package. Closes #139 --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
Why
Every open follow-up issue in coder/chat gets one small, reviewed PR, and this one closes #135. Under
DispatchDeferred, aStatepanic in the prelude unwinds tonet/http, which recovers it.dispatchDeferredfreed the admission slot only when the prelude returned, so the panicked delivery kept its slot and its queue turn record, andShutdowntimed out.What changed
dispatchDeferredfrees the slot in adeferon every exit that hands no work to a tail, a panic included. The panic still reachesnet/http.sync.OnceFuncfrom fix(runtime): harden the queue turn record against a double release and a State panic #134, so no exit can release twice.httptest.Server. It panicsState.AcquireLockunder the queue strategy andState.MarkEventunder the burst strategy.Scope
dispatchDeferred. Before the fix, aMarkEventpanic leaked the slot under drop, debounce, burst, and concurrent too.main.Tradeoffs
recoverand re-panic would also free the slot. Thedeferwith an ownership flag frees it and leaves the panic value and stack untouched.Blast Radius
Only deferred dispatch changes. A delivery that resolves in the prelude now frees its slot as
dispatchDeferredreturns, not just before, which the caller cannot tell apart.Verification
Shutdown ... context deadline exceeded. The queue case also keeps 1 turn record.recoverfails the new test with status 200. A release withoutOnceFunctrips theleaveQueueScopeassert in the queue tests. A release without the ownership flag fails two admission tests.-race.go vetandgo testpass in every workspace module, andgo test -racepasses for the root package.Closes #135
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh